Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction #13032

raynelfss · 2024-08-25T17:28:59Z

Summary

Tracked by #13001 and preceeded by #12975
The following commits aim to add a DAGCircuit::extend() to add a chain of DAGOpNodes to the DAGCircuit based on a sequence of PackedInstruction.

Details and comments

These commits add the following two methods:

DAGCircuit::extend() to add a sequence of PackedInstruction continously avoiding having to re-add each bit/var's output node each time an instruction is added. The addition of the output node is only performed once all instructions have been added correctly to the DAGCircuit.

Known issues:

At the moment, I'm still thinking of ways of handling vars correctly as we need to keep track of each bit/var's output node and vars are currently represented as PyObject which are not hashable.
- ~~One possible solution would be to use a PyDict however the structures that are being stored within this mapping are rust-native (NodeIndex, Wire)~~.
- ~~Another solution would be to somehow internalize these structures, which I'm still thinking on how to do.~~
Feel free to comment with any ideas.

Blockers

Implement a capacity-allocated constructor for DAGCircuit in Rust. #12975 (will rebase once merged)
[DAGCircuit Oxidation] Port DAGCircuit to Rust #12550

coveralls · 2024-08-25T17:53:31Z

Pull Request Test Coverage Report for Build 10740673998

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

104 of 111 (93.69%) changed or added relevant lines in 1 file are covered.
106 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.008%) to 89.155%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/dag_circuit.rs	104	111	93.69%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/optimization/split_2q_unitaries.py	1	96.77%
qiskit/transpiler/passes/routing/stochastic_swap.py	1	95.39%
crates/accelerate/src/synthesis/clifford/utils.rs	1	78.06%
crates/circuit/src/dag_node.rs	3	81.2%
crates/qasm2/src/lex.rs	3	93.23%
crates/circuit/src/packed_instruction.rs	11	93.08%
crates/circuit/src/circuit_instruction.rs	12	82.93%
crates/circuit/src/operations.rs	13	88.67%
crates/accelerate/src/results/marginalization.rs	17	90.17%
crates/circuit/src/dag_circuit.rs	44	88.92%

Totals
Change from base Build 10720509731:	0.008%
Covered Lines:	72737
Relevant Lines:	81585

💛 - Coveralls

qiskit-bot · 2024-08-26T12:30:35Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@kevinhartman
@mtreinish

ElePT · 2024-09-04T12:04:00Z

I believe that the PR doesn't have any open blocker. I will remove the on hold label. Let me know if I missed something.

- Introduce a method that adds a chain of `PackedInstruction` continuously avoiding the re-linking of each bit's output-node until the very end of the iterator. - TODO: Add handling of vars - Add header for a `from_iter` function that will create a `DAGCircuit` based on a chain of `PackedInstruction`.

- Fix incorrect re-insertion of last_node.

- Remove `from_iter`

- Use `entry` api to either modify or insert a value if missing.

- A bug that adds duplicate edges to vars has been temporarily fixed. However, the root of this problem hasn't been found yet. A proper fix is pending. For now skip those instances.

- A set collecting all the new nodes to connect with a new node was preventing additional wires to connect to subsequent nodes.

… nomenclature.

raynelfss · 2024-09-04T14:10:52Z

I believe that the PR doesn't have any open blocker. I will remove the on hold label. Let me know if I missed something.

I was about to do that :) Thank you

ElePT

Thanks @raynelfss! This PR is quite useful for transpiler passes that build new instructions (like unitary synthesis). I have been using these changes in my PR and everything has worked as expected so far, so you can have that as a "unit test" data point. I might be overseeing something but I would approve this PR after addressing a few minor (and quite superficial) comments.

crates/circuit/src/dag_circuit.rs

@ElePT

- Caught by @ElePT Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

ElePT

LGTM

ElePT

Actually, I think I have found an issue when trying to add instructions that have no clbits. When I initialize a packed instruction with clbits: target_dag.cargs_interner.get_default(), I then get a the caller is responsible for only using interner keys from the correct interner error. I will try to come up with a minimal example that reproduces the problem.

I want to make sure there are no issues for the case with empty clbits

ElePT · 2024-09-05T11:04:47Z

The issue was fixed by #13092, it was unrelated to the changes in this PR :)

kevinhartman · 2024-09-05T15:02:43Z

I can give this a quick look as well if it's helpful (I see it is not in the merge queue yet)

kevinhartman

This is looking good, thanks for adding this @raynelfss!

The one larger point I think we should start to be weary of is introducing new methods that aren't covered by any of our existing testing. We've operated in a bit of a free-for-all the last few months while porting things to Rust, under the assumption that our existing Python test code would provide sufficient coverage.

IMO, this new method is sufficiently complex and should be unit tested. I'm not sure if we've decided on an approach to exercise the Rust-only API of our pyclasss. Perhaps @mtreinish has opinions / suggestions?

crates/circuit/src/dag_circuit.rs

mtreinish · 2024-09-05T16:18:15Z

IMO, this new method is sufficiently complex and should be unit tested. I'm not sure if we've decided on an approach to exercise the Rust-only API of our pyclasss. Perhaps @mtreinish has opinions / suggestions?

In general having rust unit tests is possible and we have them already in place where it is possible/makes sense for PRs like this with new rust specific functionality. See: 86c63eb for a recent example (which is already setup to run in CI: https://dev.azure.com/qiskit-ci/qiskit/_build/results?buildId=60452&view=logs&j=b9878877-bc64-52fc-fc03-577dd0aa7cb4&t=2cc4852e-f347-585e-188c-b93a0bf20b0f&l=363 ). But what we will struggle with here is that this new method takes a py token and we don't have a mechanism to interact with python via rust unit tests.

I would agree we should have this exercised somehow in an ideal world. But I think we probably are ok to let this slide if we can't come up with a solution for unit testing with a python dependency. Given that there are several PRs actively based on top of this and those are exercising the code and tested via Python AFAICT.

- Use Entry API to modify last nodes in the var. - Build new_nodes with an allocated vec. - Add comment explaining the removal of the edge between the output node and its predecessor.

- Use `or_insert_with` instead of `or_insert` to perform actions before inserting a value.

kevinhartman

LGTM, thanks for the changes!

raynelfss mentioned this pull request Aug 26, 2024

Port circuit_to_dag to Rust #13036

Merged

6 tasks

raynelfss added on hold Can not fix yet performance Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Aug 26, 2024

raynelfss changed the title ~~[WIP] Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction~~ Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction Aug 26, 2024

raynelfss marked this pull request as ready for review August 26, 2024 12:30

raynelfss requested a review from a team as a code owner August 26, 2024 12:30

raynelfss force-pushed the rusty-dag-add-from-iter branch from 5729293 to 74d8b03 Compare August 26, 2024 18:58

raynelfss linked an issue Aug 27, 2024 that may be closed by this pull request

Create a DAGCircuit::add_from_iter() method in rust #13003

Closed

raynelfss added the priority: high label Aug 27, 2024

raynelfss force-pushed the rusty-dag-add-from-iter branch 2 times, most recently from 7056ab4 to 0288597 Compare August 30, 2024 22:50

raynelfss added this to the 1.3 beta milestone Sep 3, 2024

ElePT removed the on hold Can not fix yet label Sep 4, 2024

raynelfss added 8 commits September 4, 2024 09:54

Fix: leverage new methods in layers

a4ed753

- Fix incorrect re-insertion of last_node.

Fix: Keep track of Vars for add_from_iter

5cf7417

- Remove `from_iter`

Fix: Incorrect modification of last nodes in add_from_iter.

e6c4fff

- Use `entry` api to either modify or insert a value if missing.

Fix: Cycling edges in when adding vars.

24c8b6a

- A bug that adds duplicate edges to vars has been temporarily fixed. However, the root of this problem hasn't been found yet. A proper fix is pending. For now skip those instances.

Fix: Remove set collecting all nodes to be connected.

0821aff

- A set collecting all the new nodes to connect with a new node was preventing additional wires to connect to subsequent nodes.

Fix: Adapt to Qiskit#13033

dcf674b

Refactor: add_from_iter is now called extend to stick with Rust…

3cb950e

… nomenclature.

raynelfss force-pushed the rusty-dag-add-from-iter branch from 0288597 to 3cb950e Compare September 4, 2024 14:09

ElePT reviewed Sep 4, 2024

View reviewed changes

crates/circuit/src/dag_circuit.rs Outdated Show resolved Hide resolved

crates/circuit/src/dag_circuit.rs Outdated Show resolved Hide resolved

Fix docstring

ff4b6eb

- Caught by @ElePT Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

raynelfss and others added 2 commits September 4, 2024 12:58

Fix: Remove duplicate vars check

8011fdf

Merge branch 'main' into rusty-dag-add-from-iter

2ee5b40

ElePT previously approved these changes Sep 4, 2024

View reviewed changes

ElePT reviewed Sep 5, 2024

View reviewed changes

ElePT mentioned this pull request Sep 5, 2024

Add qubits and clbits to dag in DAGCircuit::with_capacity #13090

Closed

ElePT previously approved these changes Sep 5, 2024

View reviewed changes

Merge branch 'main' into rusty-dag-add-from-iter

2b765b7

kevinhartman reviewed Sep 5, 2024

View reviewed changes

crates/circuit/src/dag_circuit.rs Outdated Show resolved Hide resolved

crates/circuit/src/dag_circuit.rs Show resolved Hide resolved

Fix: Corrections from code review.

468869c

- Use Entry API to modify last nodes in the var. - Build new_nodes with an allocated vec. - Add comment explaining the removal of the edge between the output node and its predecessor.

raynelfss dismissed ElePT’s stale review via 468869c September 6, 2024 13:16

Fix: Improper use of Entry API.

d846cd0

- Use `or_insert_with` instead of `or_insert` to perform actions before inserting a value.

kevinhartman approved these changes Sep 6, 2024

View reviewed changes

kevinhartman added this pull request to the merge queue Sep 6, 2024

Merged via the queue into Qiskit:main with commit 3d4bab2 Sep 6, 2024
15 checks passed

raynelfss deleted the rusty-dag-add-from-iter branch September 11, 2024 12:14

raynelfss mentioned this pull request Sep 11, 2024

Add methods to more easily add operations to a DAGCircuit from Rust #13134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction #13032

Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction #13032

raynelfss commented Aug 25, 2024 •

edited

Loading

coveralls commented Aug 25, 2024 •

edited

Loading

qiskit-bot commented Aug 26, 2024

ElePT commented Sep 4, 2024

raynelfss commented Sep 4, 2024

ElePT left a comment

ElePT left a comment

ElePT left a comment

ElePT commented Sep 5, 2024

kevinhartman commented Sep 5, 2024

kevinhartman left a comment

mtreinish commented Sep 5, 2024

kevinhartman left a comment

Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction #13032

Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction #13032

Conversation

raynelfss commented Aug 25, 2024 • edited Loading

Summary

Details and comments

Known issues:

Blockers

coveralls commented Aug 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10740673998

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

qiskit-bot commented Aug 26, 2024

ElePT commented Sep 4, 2024

raynelfss commented Sep 4, 2024

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

ElePT commented Sep 5, 2024

kevinhartman commented Sep 5, 2024

kevinhartman left a comment

Choose a reason for hiding this comment

mtreinish commented Sep 5, 2024

kevinhartman left a comment

Choose a reason for hiding this comment

raynelfss commented Aug 25, 2024 •

edited

Loading

coveralls commented Aug 25, 2024 •

edited

Loading